Fix GH-16186: Ignore scope when compiling closure in non-tracing jit #16200
Conversation
arnaud-lb
force-pushed
the
gh16186
branch
2 times, most recently
from
56e1803 to
edc010b
Compare
arnaud-lb
changed the title
| if (JIT_G(trigger) == ZEND_JIT_ON_FIRST_EXEC) { | ||
| zend_jit_op_array_extension *jit_extension = (zend_jit_op_array_extension*)ZEND_FUNC_INFO(op_array); | ||
| op_array = (zend_op_array*) jit_extension->op_array; | ||
| } else if (JIT_G(trigger) == ZEND_JIT_ON_HOT_COUNTERS) { | ||
| zend_jit_op_array_hot_extension *jit_extension = (zend_jit_op_array_hot_extension*)ZEND_FUNC_INFO(op_array); | ||
| op_array = (zend_op_array*) jit_extension->op_array; | ||
| } else { | ||
| ZEND_ASSERT(!op_array->scope); |
There was a problem hiding this comment.
What about ZEND_FUNC_JIT_ON_PROF_REQUEST?
There was a problem hiding this comment.
I added it here initially, but I then I figured that it was not necessary because we never compile closures in ZEND_FUNC_JIT_ON_PROF_REQUEST mode (according to my understanding of zend_jit_check_funcs()). In theory we could compile closures in zend_jit_check_funcs() by also iterating over op_array->dynamic_func_defs, but in this case we would get the right op_array, so fetching it from jit_extension would not be necessary.
I considered this, but I agree it would be more error prone. I didn't see possible drawbacks of the current approach, especially since the tracing JIT does this too (although it's possibly for a different reason). |
* PHP-8.4: NEWS for GH-16200 Use original op_array when JIT compiling a Closure
Fixes GH-16186.
zend_jit()assumes that Closure op_arrays have no scope, but this is not true for all triggers. E.g. with the hot counter trigger, the op_array is a copy with a scope.As a result, we assume the class of
$thishere, but it is later changed in::bind(), which causes the issue.Here I copy what is done for the tracing JIT: Use the original op_array, which has no scope.